Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #119: Guarantee executing popOperand() in popUninitializedVariableOperand() via moving popOperand() out of "assert" #120

Closed
wants to merge 1 commit into from

Conversation

HeartSaVioR
Copy link
Contributor

There's a bug discovered in #119 which seems to execute "stateful" operation into "assert". The execution is not guaranteed, and leads to inconsistent stack map.

This patch adds new UT to reproduce #119 - it fails on latest master branch, and passes with this PR.

…nitializedVariableOperand() via moving popOperand() out of "assert"
@HeartSaVioR
Copy link
Contributor Author

It would be ideal if we run the entire tests twice via assert option on and off; not sure Janino build does that.

@HeartSaVioR
Copy link
Contributor Author

#121 to enable Travis CI build which helps to figure out such case.

@aunkrig
Copy link
Member

aunkrig commented Mar 16, 2020

Merged this PR, but (intentionally!) not the unit test, because that unit test may have side effects (setting / resetting assertion status).

@aunkrig aunkrig closed this Mar 16, 2020
@aunkrig
Copy link
Member

aunkrig commented Mar 16, 2020

It would be ideal if we run the entire tests twice via assert option on and off; not sure Janino build does that.

Strictly speaking, you're right. However this bug is a singleton, and the very small risk that "it will occur again" does not justify the effort/time to run the regression tests twice.

Alas, what is much more critical, is to run all the tests agains several JREs (at least 6, 7, 8 and 11), which is extremely difficult because JUNIT doesn't support that. I once wrote a JUNIT TestClassRunner that attempts to implement that

https://svn.code.sf.net/p/loggifier/code/de.unkrig.commons/trunk/commons-junit4/src/main/java/de/unkrig/commons/junit4/runner/MultipleJresTestClassRunner.java

, but it is terribly slow and clumsy.

So the unit tests that are executed by maven are only regarded as "sanity testing", while I do the really careful regression testing manually with ECLIPSE.

Maybe I should open a to-do issue for extending the UTs with the MultipleJresTestClassRunner.

@HeartSaVioR
Copy link
Contributor Author

HeartSaVioR commented Mar 16, 2020

Thanks for reviewing and merging!

Merged this PR, but (intentionally!) not the unit test, because that unit test may have side effects (setting / resetting assertion status).

That's OK, as it turned out other unit tests were failing based on the assertion status.

However this bug is a singleton, and the very small risk that "it will occur again" does not justify the effort/time to run the regression tests twice.

While the bug was a singleton, we had no idea what is happening, even there're a couple of 3.1.x versions released already. That's the matter of "stability" - given Janino is more likely used as a library, the users would expect more stability; showing the efforts of striving to guarantee stability has been already the first class priority in most of open source projects, and that's why other projects have been adopted various integration tools like CI, test coverage, etc.

Alas, what is much more critical, is to run all the tests agains several JREs (at least 6, 7, 8 and 11), which is extremely difficult because JUNIT doesn't support that.
... , but it is terribly slow and clumsy.

...and which is very trivial to add in Travis CI build with running in parallel.

https://travis-ci.org/github/HeartSaVioR/janino/builds/663233350

Screen Shot 2020-03-17 at 5 39 36 AM

(Not fully parallel though - looks like 5 parallel builds allowed per a trigger, so two groups of builds happen. If you reduce the JDK to 2 flavors, they would be grouped with one group, but can't we wait for just 10 mins?)

Screen Shot 2020-03-17 at 5 52 28 AM

(OpenJDK 11 seems to fail due to Javadoc issue which I'm not aware of - that can be fixed or applied workaround once we have a plan to adopt it.)

OpenJDK 10 build failure was from:

test_15_27_1__Lambda_parameters[CompilerFactory=jdk](org.codehaus.commons.compiler.tests.JlsTest)  Time elapsed: 0.02 sec  <<< ERROR!
org.codehaus.commons.compiler.CompileException: Line 1, Column 51: 'var' is not allowed here (compiler.err.var.not.allowed.here)
	at org.codehaus.commons.compiler.tests.JlsTest.test_15_27_1__Lambda_parameters(JlsTest.java:2753)

I once wrote a JUNIT TestClassRunner that attempts to implement that
https://svn.code.sf.net/p/loggifier/code/de.unkrig.commons/trunk/commons-junit4/src/main/java/de/unkrig/commons/junit4/runner/MultipleJresTestClassRunner.java

Unless the class can "detect" the path of JREs, this change would make others (including me) stop to contribute Janino or stuck to 3.0.x - I'm MacOS/IntelliJ user, and most of Spark contributors are also Linux/MacOS users. It even seems to be sensitive to bugfix version as well - if you upgrade JRE you'll be going to fix the code. Why not let the existing great tools do it for you instead?

@aunkrig
Copy link
Member

aunkrig commented Mar 17, 2020

Oh, MultipleJresTestClassRunner is in its infancy, and the hard-coded JAVA_HOME paths are certainly not to last.

However I fear that I don’t get the MJTCR running... the JUNIT APIs are more complex than you'd expect. So I'll definitely look into TRAVIS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants